Skip to content

Fix: skip pre-start timesync init for igc - #1666

Open
dmkarthi wants to merge 2 commits into
OpenVisualCloud:mainfrom
dmkarthi:mtl_dev_ptp_igc
Open

Fix: skip pre-start timesync init for igc#1666
dmkarthi wants to merge 2 commits into
OpenVisualCloud:mainfrom
dmkarthi:mtl_dev_ptp_igc

Conversation

@dmkarthi

@dmkarthi dmkarthi commented Jul 20, 2026

Copy link
Copy Markdown

Summary

This PR updates mt_dev_create() to avoid pre-start time sync initialization on IGC.
IGC requires different handling for timesync startup timing. This change prevents invoking the pre-start timesync init path for IGC while keeping existing behavior unchanged for other PF drivers.

File changed

  • lib/src/dev/mt_dev.c

Diff

In the DPDK 21.11+ pre-start timesync path, add an extra guard:

  • Before: run dev_start_timesync() for PF ports when PTP service or HW timestamp is requested.
  • After: same logic, but skip this path when inf->drv_info.drv_type == MT_DRV_IGC.
if ((mt_user_ptp_service(impl) || mt_user_hw_timestamp(impl)) &&
    (port_type == MT_PORT_PF) && (inf->drv_info.drv_type != MT_DRV_IGC)) {
  ret = dev_start_timesync(inf);
  if (ret >= 0) inf->feature |= MT_IF_FEATURE_TIMESYNC;
}
  • Minimal, one-line condition change.
  • No API surface changes.

@dmkarthi
dmkarthi marked this pull request as draft July 20, 2026 13:08
@dmkarthi dmkarthi changed the title dev: skip pre-start timesync init for igc Fix: skip pre-start timesync init for igc Jul 21, 2026
@dmkarthi

Copy link
Copy Markdown
Author

Tested with DirectView LED project by enabling PTP, observed Seg fault when ran on I225 NIC (igc kernel driver).

"dvledtx" received signal SIGSEGV, Segmentation fault.
0x00007fffee978da7 in eth_igc_timesync_enable ()
from /usr/local/lib/x86_64-linux-gnu/dpdk/pmds-26.0/librte_net_e1000.so.26.0

#0 eth_igc_timesync_enable () [librte_net_e1000.so.26.0]
#1 rte_eth_timesync_enable () [librte_ethdev.so.26]
#2 dev_start_timesync () [libmtl.so]
#3 mt_dev_create () [libmtl.so]
#4 mtl_init () [libmtl.so]
#5-#7 dvledtx application frames (mtl_tx_init → main)

These changes fix seg fault seen. Also the PR #1629 has similar changes.

@dmkarthi
dmkarthi marked this pull request as ready for review July 28, 2026 03:59

@DawidWesierski4 DawidWesierski4 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, but I think there may be a misunderstanding of what this patch is trying to achieve.
What's happening here is that the no_timesync flag is being set on the MTL side, which means this implementation is not operating on the hardware clock. Instead, it relies on TSC time with a software-adjusted delta which is essentially a best-effort PTP approximation.
This approach has inherent limitations in terms of accuracy and reliability compared to true hardware timestamping. Could you clarify the intended use case, or consider revisiting the implementation to leverage proper hardware clock support if the goal is genuine PTP compliance?

@DawidWesierski4

Copy link
Copy Markdown
Collaborator

we are working on bringing support on 225 -> this is happening rn,
So we could theoretically merge this as is for the support and then add when reliable testing and everythign will be working again

I would reather recommand clients to work with ptp synchronized system tbh
-> Synchronize the system clock outside mtl then just run the MTL workloads As is.

But i can see this as a temporary workaround if properlly documented?

@dmkarthi

dmkarthi commented Aug 11, 2026

Copy link
Copy Markdown
Author

we are working on bringing support on 225 -> this is happening rn, So we could theoretically merge this as is for the support and then add when reliable testing and everythign will be working again

I would reather recommand clients to work with ptp synchronized system tbh -> Synchronize the system clock outside mtl then just run the MTL workloads As is.

But i can see this as a temporary workaround if properlly documented?

@DawidWesierski4 when PTP is enabled in MTL and runs on I225/I226 hardware we see the Segmentation fault, this change avoids it. PR #1629 also has similar fixes.

@DawidWesierski4

Copy link
Copy Markdown
Collaborator

we are working on bringing support on 225 -> this is happening rn, So we could theoretically merge this as is for the support and then add when reliable testing and everythign will be working again
I would reather recommand clients to work with ptp synchronized system tbh -> Synchronize the system clock outside mtl then just run the MTL workloads As is.
But i can see this as a temporary workaround if properlly documented?

@DawidWesierski4 when PTP is enabled in MTL and runs on I225/I226 hardware we see the Segmentation fault, this change avoids it. PR #1629 also has similar fixes.

Understandable but my point still stands,
this WA would make it so that on the hardware that supports PTP hardware akcelerations we are using software ptp calculation

please comment above that TODO unlock and add a warning when this case is triggered

documentation about VF's is takling about it

I Can even merge it as is and just add warnings in separete PR if you are pressed for time

@DawidWesierski4

DawidWesierski4 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Hi i wanted to add some testing and other functionality so i cherry picked your changes into my branch
#1677

i preserved the auther and plan to rebase the chagne so the change will have your name on it but i changed them tad bit so it passes our doc/coding-standard.md

please review it, and don't hesitate to be brutal about feedback

@dmkarthi

dmkarthi commented Aug 12, 2026

Copy link
Copy Markdown
Author

we are working on bringing support on 225 -> this is happening rn, So we could theoretically merge this as is for the support and then add when reliable testing and everythign will be working again
I would reather recommand clients to work with ptp synchronized system tbh -> Synchronize the system clock outside mtl then just run the MTL workloads As is.
But i can see this as a temporary workaround if properlly documented?

@DawidWesierski4 when PTP is enabled in MTL and runs on I225/I226 hardware we see the Segmentation fault, this change avoids it. PR #1629 also has similar fixes.

Understandable but my point still stands, this WA would make it so that on the hardware that supports PTP hardware akcelerations we are using software ptp calculation

please comment above that TODO unlock and add a warning when this case is triggered

documentation about VF's is takling about it

I Can even merge it as is and just add warnings in separete PR if you are pressed for time

Even if user enable PTP on the hardware which doesn't support PTP(ex. I225- kernel driver IGC) should not cause any failure from MTL side(like seg fault), instead it should give a WARN message and switch to software PTP calculation.
This PR change was made just to avoid seg fault which we observed on I225.

Feel free to fix which holds good as per MTL codeflow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants